Stop failed logins from revealing which usernames exist - #439

Fixes #240.

A failed login gave away whether the username was real, in two separate ways. This fixes both, and adds a server event so an admin can still see when an account locks.

The messages

The lockout paths in DefaultUserController.authorizeUser named the account and reported how long the lock had left to run, and a wrong password for a real account reported the attempts left before the lock. An unauthenticated caller could tell a valid username from an invalid one. A wrong password, an unknown username and a locked account now all return FAIL and Login failed.

Per @tonygermano, the message doesn't say "incorrect username or password", because for a locked account given the right password that isn't true. There is no property to bring the detailed messages back.

The lockout event

Per @tonygermano, the failed login that locks an account dispatches a User account locked server event, level WARNING, outcome FAILURE, with the username and the time it unlocks (locked until). Attempts against a locked account are rejected before a strike is counted, so there is one event per lock.

It is dispatched from DefaultUserController, so it covers HTTP basic auth on the API too, which goes through MirthServlet and logs no Login event at all. It carries no IP address, since the controller doesn't have one; the Login event logged for the same /users/_login request has it.

On a running engine with retrylimit = 30, the 31st wrong password logged one User account locked event with username and a locked until an hour later. The 32 attempts after it against the locked account, the right password included, logged no more. Basic auth on GET /api/users/current did the same, and there the lock event was the only event the 33 failed requests left.

The timing

Fixing the messages alone left a second and much larger signal in place. Closing #240 on the message fix by itself would have overstated what changed.

The password digest is only reached for a valid, unlocked user. An unknown username never enters the validUser != null block, and a locked account returns before it, so both skipped 600,000 PBKDF2 iterations. Measured with generic messages already enabled, 250 samples per cohort:

The distributions do not overlap. The fastest unlocked sample was still 240x slower than the slowest locked one, so one timed request classified any username whatever the response body said. An attacker could enumerate accounts without reading a message at all.

A same-account control confirms the cause. An account that crossed the strike limit partway through a run stepped from 232.03 ms to 0.33 ms at the boundary, with no samples in between. The account did not change, only its lockout state did.

Every path that skips the real digest now runs a throwaway one and discards the result: an unknown username, a locked account, an account that exists but has never had a password set, and a failed pre-2.2 hash check. The last two are reachable in normal use, since an admin can create an account before provisioning it and legacy hashes survive upgrades. The throwaway goes through matches() so the fallback digester runs the way it does on the real path, and the hash is generated once from the live digester so it follows the configured algorithm and iteration count. Measured again on the same rig by the same method:

The no-password case was checked separately against a real account and came back at 234.16 ms against 233.89 for a valid user and 233.45 for an unknown one.

Rechecked on this commit on a slower machine, where one digest costs about 1.25 s, with 30 interleaved samples each: unknown username 1237.75 ms, valid + unlocked 1251.88 ms, valid + locked 1250.89 ms.

This is not constant time and I am not claiming it is. A valid user still costs one extra credentials query that a miss does not, which measured under a millisecond against a 237 ms floor. There is a ponytail: marker on the helper naming that ceiling and the condition for revisiting it.

There is a tradeoff here worth arguing with. Locked accounts are no longer cheap to reject, so lockout no longer caps the cost of repeatedly probing an account already known to exist. My reasoning is that an attacker gains nothing by locking an account once every username costs the same, so that cheapness was mostly helping the attacker. Tell me if you see it differently.

What this does not cover

An AuthorizationPlugin returning a non-null LoginStatus returns before any of this code, so OIDC, LDAP and similar plugins are unaffected in both directions: nothing here breaks them, and nothing here protects them. Their messages and timing are their own. A plugin that replaces UserController outright through ExtensionLoader would miss this entirely, and grepping this repo will not reveal one.

Left for their own issues: a failure reason on every failed Login event (the reason is known in the controller, but the Login event is built in the servlet from the same LoginStatus the caller receives), and lock status in the user screens plus a REST unlock.

Tests

DefaultUserControllerAuthorizeUserTest covers what a failed login returns and the event it dispatches: a locked account gets the generic failure and is indistinguishable from an unknown username, a wrong password for a real account is indistinguishable from an unknown username, and the attempt that locks an account dispatches one User account locked event while the attempts after it dispatch none. All four fail against current main and pass with this change.

It binds mock ControllerFactory and SqlConfig through Guice static injection, the way DefaultHttpConfigurationTest already does, so no database is involved. Lockout state is set directly on the User, because LoginRequirementsChecker reads the strike count and last strike time straight off it.

No test asserts the timing. Mocking the digester removes the cost being measured, and a real one makes every call take 230 ms and turns the suite into a flaky performance test. The numbers above come from a running server.

Not added: a smoke test in ci/. The new harness boots a real engine across seven configurations and has a Java seam with a live Client, so the message behaviour could be asserted there against all five databases. That is worth doing and I have not done it; it belongs in its own change rather than growing this one further. The timing assertion would still be a poor fit for shared runners.

Clean build, 685 tests, 0 failures.

Credit

This is @mgaffigan's fix from #241, reworked for the current source layout. That branch predates the June move to src/main/java and no longer applies. The timing half and the lockout event are new.

Test rename

PasswordRequirementsTests becomes PasswordRequirementsTest. The build includes **/*Test.class only, so the plural-named class has never run in any build. Six dormant cases now execute and pass. The dead *Tests problem is wider than this file and needs its own issue.

Behavior change

Every failed login now says Login failed., where it used to say Incorrect username or password. and, with lockout enabled, add the attempts left or the lock and its remaining time. Anything scraping the message string will notice, and a locked-out user is no longer told they are locked; the admin sees the event instead. Raised on #241 by me and @ChristopherSchultz.

How to verify by hand

Defaults are retrylimit = 100 and lockoutperiod = 1. That period is in hours rather than minutes, per Duration.standardHours at LoginRequirementsChecker:99.

1. POST /api/users/_login with a real username and a wrong password, 101 times. The 101st locks the account
2. Send the same request for a nonexistent username, and for the locked account with its correct password. All three responses are byte-identical: FAIL, Login failed.
3. GET /api/events?name=User%20account%20locked shows one event for the account, with username and locked until
4. Time the three cases. They come within about 1% of each other instead of hundreds of times apart

Co-authored with Claude Code.